Skip to content

Conversation

@fuheaven
Copy link

Motivation and Context

This change fixes a critical URL parsing bug in the MCP (Model Context Protocol) client's SSE (Server-Sent Events) endpoint handling. The original implementation used urljoin(url, sse.data) directly, which incorrectly handled absolute paths in sse.data, causing the client to access wrong endpoints.
Problem scenario:
SSE URL: https://example.com/agent-5o/mcp-stage/sse
sse.data: /messages/
Original result: https://example.com/messages/ (incorrect - loses path prefix)
Fixed result: https://example.com/agent-5o/mcp-stage/messages/ (correct - preserves path prefix)
This bug prevented MCP clients from establishing proper connections to servers deployed with path prefixes or behind reverse proxies.

Tested SSE URLs with various path prefix scenarios. Verified correct parsing of both relative and absolute paths in sse.data
Confirmed proper endpoint URL construction and successful connections. Validated backward compatibility with existing deployments. Tested edge cases including URLs with and without trailing slashes

Additional context

Core Implementation Logic:

Parse Original SSE URL: Use urlparse() to analyze the structure of the SSE connection URL
Construct Base URL:
If the path ends with '/', use the original URL as base
Otherwise, remove the last path segment (typically 'sse') while preserving the directory path
Safe URL Joining: Use urljoin(base_url, sse.data.lstrip('/')) to ensure correct relative path resolution

@pja-ant
Copy link
Contributor

pja-ant commented Sep 19, 2025

I could be missing something here, but isn't the current behavior correct? The path /messages/ is an absolute path, so it's intentional that removes the path prefix. If you want a relative path, then use messages/ and urljoin will do the right thing.

@fuheaven
Copy link
Author

I could be missing something here, but isn't the current behavior correct? The path /messages/ is an absolute path, so it's intentional that removes the path prefix. If you want a relative path, then use messages/ and urljoin will do the right thing.

You're absolutely right about urljoin()'s technical behavior. However, the issue we're addressing is that we can't control the format of sse.data that comes from the server response.

In our case, the server might return either '/endpoint' or 'endpoint' in sse.data, and we need to handle both cases reliably to construct the correct endpoint URL relative to the original MCP server path.

@pja-ant
Copy link
Contributor

pja-ant commented Sep 22, 2025

Is this summary correct?

  • The behavior of the client SDK is in fact correct here (i.e. treating absolute paths as absolute).
  • The problem is that you are working with servers that are returning absolute paths but actually want you to use a relative path.
  • You do not have control over those servers

My sense is that this is a bug in the servers and ideally should be fixed there since some servers may be intentionally returning absolute paths and actually want you to use an absolute path.

@felixweinberger felixweinberger added needs confirmation Needs confirmation that the PR is actually required or needed. bug Something isn't working labels Sep 22, 2025
@fuheaven
Copy link
Author

Is this summary correct?

  • The behavior of the client SDK is in fact correct here (i.e. treating absolute paths as absolute).
  • The problem is that you are working with servers that are returning absolute paths but actually want you to use a relative path.
  • You do not have control over those servers

My sense is that this is a bug in the servers and ideally should be fixed there since some servers may be intentionally returning absolute paths and actually want you to use an absolute path.

You're correct, and thank you for that clear analysis. The servers we're dealing with are returning absolute paths when they should return relative paths, and this is fundamentally a server-side bug that should ideally be fixed there.

@pja-ant
Copy link
Contributor

pja-ant commented Sep 23, 2025

Thanks for clarifying -- seems we are on the same page. Since that's the case, I would recommend trying to get in touch with the server author to fix the server to return the correct paths. I don't think we can make this change as, while it would fix things in your case, it can break properly-behaving servers by ignoring their request for an absolute path.

I'll close this for now since I don't think we can break things for well-behaved servers.

@pja-ant pja-ant closed this Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs confirmation Needs confirmation that the PR is actually required or needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants